Skip to content

fix: use SameValueZero equality for special numeric values - #1573

Open
yslpn wants to merge 7 commits into
open-circle:mainfrom
yslpn:fix/same-value-zero-equality
Open

fix: use SameValueZero equality for special numeric values#1573
yslpn wants to merge 7 commits into
open-circle:mainfrom
yslpn:fix/same-value-zero-equality

Conversation

@yslpn

@yslpn yslpn commented Aug 2, 2026

Copy link
Copy Markdown
Member

Fix SameValueZero equality for special numeric values

Summary

This change makes value equality consistent across literal, intersect,
value, notValue, values, and notValues.

Problem

JavaScript strict equality and ordering comparisons do not consider NaN equal
to itself. As a result, several APIs rejected a value that was identical to its
configured requirement:

  • literal(NaN) rejected NaN.
  • value(NaN) and values([NaN]) rejected NaN.
  • notValue(NaN) and notValues([NaN]) accepted NaN.
  • intersect reported a conflict when two branches produced NaN or invalid
    Date values for the same entry.

Changes

  • Adds the internal _sameValueZero utility.
  • Uses it in literal and _merge.
  • Uses it before the existing ordering fallback in value, notValue,
    values, and notValues.
  • Preserves the existing behavior for -0 and +0.
  • Preserves cross-instance comparison of valid Date values in the value
    actions.

Tests

Adds regression coverage for:

  • NaN matching itself in every affected API.
  • -0 and +0 remaining interchangeable.
  • Merging two invalid Date objects.

Verification

  • pnpm -C library test — 520 test files and 4514 tests passed.
  • pnpm -C library lint — passed without errors.
  • pnpm -C library format.check — passed.

Acknowledgements

This consolidated fix builds on the work and investigation in:

Closes #1477
Closes #1517
Closes #1529
Closes #1535

Thank you all for identifying the edge cases, proposing fixes, and adding
regression coverage that informed this unified change.

Summary by CodeRabbit

  • Bug Fixes

    • Improved value validation for special numeric values, including NaN and positive or negative zero.
    • Corrected literal, value, and collection comparisons for invalid dates and other edge cases.
    • Preserved special values correctly when combining data.
  • Tests

    • Added coverage for NaN, signed zero, invalid dates, and value-matching behavior across validation and comparison scenarios.

Copilot AI review requested due to automatic review settings August 2, 2026 21:52
@dosubot dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. fix A smaller enhancement or bug fix labels Aug 2, 2026
@coderabbitai

coderabbitai Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c3bbbfae-3d2a-463f-b03b-abc97cfb7f76

📥 Commits

Reviewing files that changed from the base of the PR and between d6b87da and 3ded0bc.

📒 Files selected for processing (2)
  • library/src/utils/_isValueRequirementMatch/_isValueRequirementMatch.test.ts
  • library/src/utils/_isValueRequirementMatch/_isValueRequirementMatch.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • library/src/utils/_isValueRequirementMatch/_isValueRequirementMatch.ts
  • library/src/utils/_isValueRequirementMatch/_isValueRequirementMatch.test.ts

Walkthrough

Added _isSameValueZero and _isValueRequirementMatch utilities. Updated value actions to use centralized requirement matching. Updated literal validation and intersection merging to use SameValueZero comparisons. Added tests for NaN, signed zero, equivalent values, and invalid dates.

Possibly related PRs

Suggested reviewers: fabian-hiller

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the primary change: applying SameValueZero equality to special numeric values.
Linked Issues check ✅ Passed The changes fix NaN matching, literal validation, intersect merging, invalid dates, signed zero, and add regression tests for [#1477], [#1517], [#1529], and [#1535].
Out of Scope Changes check ✅ Passed All implementation and test changes directly support the linked equality fixes and their regression coverage.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@pkg-pr-new

pkg-pr-new Bot commented Aug 2, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/valibot@1573

commit: 3ded0bc

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@library/src/schemas/intersect/utils/_merge/_merge.test.ts`:
- Around line 43-49: Strengthen the invalid-date test around _merge by
explicitly asserting that the merged result value is a Date before validating
its invalid timestamp with Number.isNaN. Keep the existing issue expectation and
timestamp assertion unchanged.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c47477ef-e073-4b0a-abe3-984fe7af8d25

📥 Commits

Reviewing files that changed from the base of the PR and between e789528 and f82b648.

📒 Files selected for processing (16)
  • library/src/actions/notValue/notValue.test.ts
  • library/src/actions/notValue/notValue.ts
  • library/src/actions/notValues/notValues.test.ts
  • library/src/actions/notValues/notValues.ts
  • library/src/actions/value/value.test.ts
  • library/src/actions/value/value.ts
  • library/src/actions/values/values.test.ts
  • library/src/actions/values/values.ts
  • library/src/schemas/intersect/utils/_merge/_merge.test.ts
  • library/src/schemas/intersect/utils/_merge/_merge.ts
  • library/src/schemas/literal/literal.test.ts
  • library/src/schemas/literal/literal.ts
  • library/src/utils/_sameValueZero/_sameValueZero.test.ts
  • library/src/utils/_sameValueZero/_sameValueZero.ts
  • library/src/utils/_sameValueZero/index.ts
  • library/src/utils/index.ts

Comment thread library/src/schemas/intersect/utils/_merge/_merge.test.ts

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes edge-case equality behavior by introducing a shared SameValueZero comparator and using it across literal/value-based validations and intersect merging, so that NaN (and invalid Date timestamps during merge) can match itself while keeping -0/+0 interchangeable.

Changes:

  • Added internal _sameValueZero utility (SameValueZero semantics: NaN equals NaN, -0 equals +0) with tests and exported it from the utils barrel.
  • Updated literal, value, notValue, values, and notValues to use _sameValueZero before (or instead of) existing comparison logic.
  • Updated intersect’s _merge to use _sameValueZero for both direct equality and Date timestamp equality (including invalid dates), with regression tests.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.

Show a summary per file
File Description
library/src/utils/index.ts Exports the new internal _sameValueZero utility.
library/src/utils/_sameValueZero/index.ts Adds barrel export for the _sameValueZero implementation.
library/src/utils/_sameValueZero/_sameValueZero.ts Introduces SameValueZero comparison helper used across multiple APIs.
library/src/utils/_sameValueZero/_sameValueZero.test.ts Adds unit coverage for NaN and signed-zero behavior.
library/src/schemas/literal/literal.ts Uses _sameValueZero so literal(NaN) works while keeping -0/+0 interchangeable.
library/src/schemas/literal/literal.test.ts Adds regression tests for NaN and signed-zero literals.
library/src/schemas/intersect/utils/_merge/_merge.ts Uses _sameValueZero for merge equality, including invalid Date timestamps.
library/src/schemas/intersect/utils/_merge/_merge.test.ts Adds regression tests for NaN merges, signed zeros, and invalid dates.
library/src/actions/values/values.ts Adds _sameValueZero short-circuit to correctly match NaN in values.
library/src/actions/values/values.test.ts Adds regression test ensuring values([NaN]) accepts NaN.
library/src/actions/value/value.ts Adds _sameValueZero short-circuit to correctly match NaN in value.
library/src/actions/value/value.test.ts Adds regression test ensuring value(NaN) accepts NaN.
library/src/actions/notValues/notValues.ts Adds _sameValueZero short-circuit so notValues([NaN]) correctly rejects NaN.
library/src/actions/notValues/notValues.test.ts Adds regression test ensuring NaN is rejected for notValues([NaN]).
library/src/actions/notValue/notValue.ts Adds _sameValueZero short-circuit so notValue(NaN) correctly rejects NaN.
library/src/actions/notValue/notValue.test.ts Adds regression test ensuring NaN is rejected for notValue(NaN).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 16 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread library/src/schemas/intersect/utils/_merge/_merge.test.ts Outdated
@fabian-hiller

Copy link
Copy Markdown
Member
  • Is this a breaking change for some of the modified functions? Can this be released as part of the next v1 release.
  • I recommend _sameValueZero follows the _is prefix that we use for functions that return a boolean.

@ysknsid25 ysknsid25 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your work!
I left a comment that was more like a personal impression 🙏

Comment thread library/src/actions/notValue/notValue.ts Outdated
@dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Aug 3, 2026
@yslpn

yslpn commented Aug 3, 2026

Copy link
Copy Markdown
Member Author
  • Is this a breaking change for some of the modified functions? Can this be released as part of the next v1 release.

I consider it a bug fix rather than an API-breaking change. With strict equality, NaN could never match itself, making value(NaN) impossible to satisfy and inverting the expected behavior of the negative actions. The change also aligns these APIs with existing picklist, which already uses SameValueZero semantics via Array.prototype.includes. There are no API or type-level changes. I am comfortable releasing it in the next v1 release. 1.5 for example.

  • I recommend _sameValueZero follows the _is prefix that we use for functions that return a boolean.

Done

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 15 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

@ysknsid25
ysknsid25 self-requested a review August 3, 2026 23:46
ysknsid25
ysknsid25 previously approved these changes Aug 3, 2026
@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Aug 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix A smaller enhancement or bug fix lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants